test: add proxy burn-in tooling - #455

Review: Correctness at ff0243ba

The two follow-up commits fix the original aggregate type mismatch, add an automated encrypted soak, and verify ciphertext at rest. I rechecked the remaining findings against the new head; these still need attention.

Blocking

1. Release artifact discovery still ignores Cargo configuration — src/soak.rs:223-253 [repro]

cargo build respects CARGO_TARGET_DIR, build.target-dir, and configured build targets, but spawn_release_proxy() always opens workspace/target/release/cipherstash-proxy. On a shared-target setup, the run either fails after building or launches a stale workspace binary.

Consume Cargo’s JSON compiler-artifact.executable instead of reconstructing the path. That identifies the artifact produced by this exact build and handles target triples as well as custom target directories.

2. An existing listener on 6432 still makes soak measure a dead process — src/soak.rs:63-116, src/database.rs:183-193 [repro]

The child is reduced to a PID before readiness and is never checked again. With another Proxy already listening, the newly built child exits with Address already in use, readiness and fixture setup use the existing Proxy, and RSS sampling targets the dead child. I reproduced a successful run with 1,701 CRUD cycles, zero final RSS, and zero reported growth.

Preflight the configured bind address, but do not treat that as proof of identity—it remains racy. Pass the Child into the run loop, check try_wait() throughout startup, migration, sampling, and worker completion, and reject zero RSS samples.

3. A timed soak can run indefinitely and runtime failures can erase its report — src/soak.rs:89-136, src/database.rs:183-193

Connection, startup-handshake, readiness-query, and CRUD futures have no deadlines. Once the sampling deadline passes, join_next() can wait forever for a wedged worker.

Sampling and worker errors also propagate before the report is written. Add bounded readiness attempts, per-operation timeouts, and a bounded worker-shutdown period. Preserve partial samples and record the terminal error before returning failure.

4. Database credentials are exposed in help and errors — src/main.rs:31-50, src/database.rs:17-20,183-190 [repro]

Clap prints the complete BURN_IN_*_DATABASE_URL environment value in --help, and connection/readiness errors interpolate the same URL.

Use hide_env_values = true and a parsed connection type with a redacted display form. Never include raw database URLs in diagnostics or reports.

Should fix

5. The aggregate assertion still panics on its intended failure case — src/conformance.rs:86-97

The new ::bigint cast fixes the unconditional numeric decoding panic. However, if the join loses every row, sum(...)::bigint is NULL and get::<_, i64>() still panics before "joined CRUD result was corrupted" can fire. Decode with try_get::<_, Option<i64>>() and assert Some(4_998).

6. Concurrent runs can deadlock or invalidate one another — migrations/0001_schema.sql, migrations/0002_seed.sql:3-5, src/soak.rs:156-219

Every run drops and recreates the public fixture tables. The remaining TRUNCATE order is also the reverse of the CRUD insert order. Reordering prevents that specific lock inversion, but concurrent runs would still destroy or contaminate each other’s fixtures and measurements.

Acquire a run-level advisory lock and retain its connection for the entire conformance or soak run; a migration-only lock is insufficient.

7. Report and gate semantics remain inconsistent — src/soak.rs:113-153

A worker error already short-circuits at join_next(), so ensure!(report.errors == 0) cannot observe one. Zero completed cycles can pass, output-path errors are discovered only after the workload, and soak passed prints before the RSS gate—I reproduced it printing success immediately before exiting 1.

Require at least one completed cycle, include terminal status in partial reports, preflight output with a sibling temporary file and atomic rename, and print success only after all gates pass.

8. The migrated database may not be the spawned Proxy’s upstream — src/soak.rs:61-74, src/soak.rs:247-253

--direct-database-url selects the database where EQL is installed and ciphertext is inspected, but the child Proxy is spawned without database arguments and reads ambient CS_DATABASE__*. An override can therefore migrate one database while the child serves another.

Pass or validate the child’s upstream configuration. Record sanitized provenance in the report: artifact hash or commit, concurrency, timestamp, actual elapsed duration, and a redacted database identity.

Additional notes

• The new CI job closes the encryption-path and execution gaps. It runs only soak and omits --max-rss-growth-mib, so deterministic conformance and retained-growth gating remain local-only. Adding them would strengthen coverage once a stable threshold is established.
• First-to-last RSS delta matches the README’s “retained growth” wording. The immediate first tick is still a cold baseline; add a warm-up or delayed first sample. Trend fitting is optional rather than a correctness requirement.
• Add kill_on_drop(true) and signal handling so panics and interrupts do not leave an owned child or discard all samples.
• Make the wide-text assertion exact, use checked multiplication for --max-rss-growth-mib, and establish a fixture migration/version strategy before the schema evolves again.
• The updated package’s three unit tests pass. The live subcommands remain the only tests of lifecycle, encryption, and report behavior.

Separately, commits 5c72f021, 31836df6, d64fcba4, and ff0243ba lack the repository-required DCO sign-off, and src/main.rs:61 should say “connections,” not “sessions.”

Review: burn-in tooling at c5cc4f44

I re-verified every finding against the code and against completed CI runs. Two candidate
findings did not hold and are listed under "Checked and cleared".

Should fix

1. The burn-in builds Proxy in release mode two times — .github/workflows/test.yml:93-97, packages/cipherstash-proxy-burn-in/src/soak.rs:306-326

mise run proxy:up calls build:binary, which builds with --target x86_64-unknown-linux-gnu
(mise.toml:680). build_release_proxy() builds the same package with no --target. The two
builds write to different directories and share no artifacts.

Each build takes about 3 minutes 45 seconds. The release profile sets codegen-units = 1
(Cargo.toml:35-37). The cache does not help: a warm run of the same build took 3 minutes 39
seconds. Earlier burn-in runs took 4 to 6 minutes, and the new conformance step has not yet run in
CI. With that step the job needs about 10 minutes of the 15-minute budget.

Pass the same --target in build_release_proxy(), or start the soak from the binary that
build:binary already produced. Do not increase timeout-minutes.

2. schema_changed is a write-once latch — packages/cipherstash-proxy/src/postgresql/context/mod.rs:565-575

set_schema_changed() only writes true. No code writes false. After a connection sends any DDL,
reload_schema_if_changed() therefore becomes an unconditional reload for the rest of that
connection's life.

Each reload sends ReloadCommand::DatabaseSchema and awaits the response (context/mod.rs:771-789).
The handler reloads the schema and the encrypt config (proxy/mod.rs:116-120). One global task
serves all connections, so these reloads become serial across connections.

This defect is already on main: the same unguarded reload runs for non-passthrough connections
(main's backend.rs:283-285) and for Code::Sync (frontend.rs:303). This PR adds one more case.
A psql session against a database with no encrypted columns now pays a schema reload and an
encrypt-config reload on every statement after its first DDL.

Do not revert backend.rs:175-181; that block fixes a real problem. Clear the flag after a
successful reload. An AtomicBool::swap(false, ...) also removes the read-then-reload race.

3. The burn-in CI job cannot reach the new passthrough branch — .github/workflows/test.yml:88-90

postgres:setup applies tests/sql/schema.sql, which creates EQL-domain columns
(tests/sql/schema.sql:38-62). Proxy infers the encrypt config from the schema
(proxy/encrypt_config/manager.rs:88-91). Each connection snapshots that config when it opens, and
is_passthrough() reads the snapshot (context/mod.rs:798-800). The soak Proxy therefore starts
with a non-empty config, and backend.rs:179-181 never runs in that job.

The unit test passthrough_reloads_changed_schema_on_ready_for_query (backend.rs:921-948) does
cover the branch. No test covers the full path from a bare database. The comment at
database.rs:176-178 states the DDL round trip works "including when this database had no encrypted
columns at boot", and nothing proves that end to end.

Run one burn-in against a database that has no encrypted columns at boot.

Nits

• soak.rs:141-146 gives the whole bootstrap the 10-second per-operation budget, but
  conformance.rs:22 gives migrate no timeout. Measured migration time in CI is about 300
  milliseconds, so the budget is safe today. Make the two paths consistent and give the migration its
  own named constant.
• tests/sql/eql-domains-uninstall.sql:14 drops each domain without IF EXISTS. All 52
  public.eql_v3_* domains are AS jsonb, so CASCADE cannot reach a sibling domain and the loop
  cannot fail today. If it ever fails, the DO block rolls back every drop, and psql exits 0
  because no task sets ON_ERROR_STOP. The teardown would then leave stale domains and report
  success. Add IF EXISTS, and set ON_ERROR_STOP=1 on the teardown task.
• On the worker-shutdown-timeout path, soak.rs:213 returns before soak.rs:221-222 refreshes the
  counters. The report can show operation and error counts that are up to 16 seconds old.
  refresh_rss_summary() solves this for RSS; the counters have no equivalent.
• Item 7 of my earlier review is still open. ensure!(report.errors == 0) (soak.rs:414) cannot
  observe an error: both increments are followed by return Err(...) (soak.rs:173-174,
  soak.rs:180-181), and the error propagates at soak.rs:223 before validate_report runs. The
  errors field itself is useful, because the sampling loop copies it into the report and the report
  is written on the failure path. Keep the field. The assertion is harmless as an invariant guard, so
  no action is needed unless you make worker errors non-fatal.

Checked and cleared

• conformance.rs:125 expect_err(...): the message gives meaningful context, which matches
  CLAUDE.md. The exit status and the skipped checks are the same as with an anyhow error.
• ids cannot overflow i32. It starts at 1,000,000, and i32::try_from returns an error instead of
  wrapping. The limit is about 2.1e9 cycles.
• Sampling and worker deadlines are correctly ordered. A final CRUD cycle can run up to
  OPERATION_TIMEOUT (10 s) past the deadline, and WORKER_SHUTDOWN_TIMEOUT is 15 s.
• No report is written when the run fails before soak.rs:88. preflight_output() and
  if-no-files-found: warn handle this deliberately.
• The ZeroKMS and CTS handshake runs at Proxy startup (proxy/mod.rs:58-62), so READY_TIMEOUT
  covers it, not the migration timeout. Measured init time was 667 ms.
• find_proxy_artifact, the atomic report write, kill_on_drop plus run_until_interrupted, and
  the RSS growth and peak helpers all read correctly.

Review — verified against head d54fdfd6

My earlier pass ran against a stale local checkout (c5cc4f44). That checkout is a divergent branch, not an ancestor of this PR, so several findings cited code that this PR never contained. I have re-verified every finding against the PR head. I withdraw five findings at the end of this review.

Must fix

1. Extended-protocol DDL does not reload the schema

packages/cipherstash-proxy/src/postgresql/frontend.rs:303

bc94d74e made the schema-changed flag a consuming read. That fixes the sticky flag I raised earlier, but it exposes an ordering defect in the frontend.

Proxy sets the flag in parse_handler (frontend.rs:846), when it parses the statement. Proxy consumes the flag in rewrite (frontend.rs:303), when the client Sync arrives. take_schema_changed clears the flag as it reads it.

A client that uses the extended protocol prepares the statement first. tokio_postgres shows the shape clearly: ToStatement for str always calls client.prepare(), and prepare sends Parse, Describe and Sync (prepare.rs:128-129). So the client sends two exchanges:

1. Parse / Describe / Sync — Proxy sets the flag at Parse, then consumes it at Sync and reloads. The DDL has not run yet. The reload reads a catalog without the new table.
2. Bind / Execute / Sync — the DDL runs. The flag is already clear, so Proxy does not reload.

The backend ReadyForQuery (backend.rs:294) also finds a clear flag. Proxy therefore misses the DDL until the background reload, 60 seconds later by default.

Before this PR the flag was sticky, so the next statement reloaded again after the DDL. That behaviour hid the defect. The take semantics remove it. This also defeats the guarantee that 8661b7df states in its own comment, for every extended-protocol client.

Affected: JDBC in prepared mode, tokio_postgres query/execute, sqlx.
Not affected: the simple query protocol, which sends no Sync. The burn-in uses the simple query protocol only, so the new tests cannot find this.

Suggested fix: reload on the backend ReadyForQuery only, and remove the frontend.rs:303 call. The backend path runs after the statement completes, so it reads a catalog that contains the DDL.

Regression test

Add this to packages/cipherstash-proxy-integration/src/schema_change.rs. The connection that runs the DDL cannot show the defect, because collect_ddl adds the table to that connection's own editable TableResolver. Only a later connection reads the reloaded global state, so the test opens a second connection.

use crate::common::{connect_with_tls, query_direct_by, random_id, trace, PROXY};
use tokio_postgres::Client;

/// Drops a fixture table through Proxy.
async fn drop_table(client: &Client, table: &str) {
    client
        .simple_query(&format!("DROP TABLE IF EXISTS {table}"))
        .await
        .unwrap();
}

/// Reads the stored value directly from PostgreSQL and asserts that Proxy
/// encrypted it. A passed-through statement stores plaintext, which the
/// `eql_v3_text_search` domain rejects, so this also proves that Proxy
/// mapped the statement instead of forwarding it unchanged.
async fn assert_stored_ciphertext(table: &str, id: i64, plaintext: &str) {
    let sql = format!("SELECT encrypted_text::text FROM {table} WHERE id = $1");
    let stored: Vec<String> = query_direct_by(&sql, &id).await;

    assert_eq!(stored.len(), 1, "expected exactly one row in {table}");
    assert_ne!(
        stored[0], plaintext,
        "value in {table}.encrypted_text was stored as plaintext"
    );
}

#[tokio::test]
async fn extended_protocol_ddl_reloads_schema_for_later_connections() {
    trace();

    let id = random_id();
    let table = format!("schema_reload_extended_{id}");
    let plaintext = "reload-after-extended-ddl".to_string();

    let ddl_client = connect_with_tls(*PROXY).await;

    // Extended protocol: Parse/Describe/Sync, then Bind/Execute/Sync.
    let sql = format!(
        "CREATE TABLE {table} (
            id bigint PRIMARY KEY,
            encrypted_text eql_v3_text_search
        )"
    );
    ddl_client.execute(&sql, &[]).await.unwrap();

    // Open the connection immediately, so that the 60-second background
    // reload cannot hide the defect.
    let client = connect_with_tls(*PROXY).await;

    let sql = format!("INSERT INTO {table} (id, encrypted_text) VALUES ($1, $2)");
    let result = client.execute(&sql, &[&id, &plaintext]).await;

    assert!(
        result.is_ok(),
        "Proxy did not reload the schema after extended-protocol DDL, \
         so the new connection cannot map {table}.encrypted_text: {:?}",
        result.err()
    );

    assert_stored_ciphertext(&table, id, &plaintext).await;

    drop_table(&ddl_client, &table).await;
}

/// Control. The simple query protocol sends no `Sync`, so the frontend never
/// consumes the flag. The backend consumes it at `ReadyForQuery`, after the
/// DDL has run, and the reload therefore reads the new table.
///
/// This test passes before and after the fix. It fails if a fix removes the
/// reload instead of moving it.
#[tokio::test]
async fn simple_protocol_ddl_reloads_schema_for_later_connections() {
    trace();

    let id = random_id();
    let table = format!("schema_reload_simple_{id}");
    let plaintext = "reload-after-simple-ddl".to_string();

    let ddl_client = connect_with_tls(*PROXY).await;

    // Simple query protocol: one Query message, no Sync.
    let sql = format!(
        "CREATE TABLE {table} (
            id bigint PRIMARY KEY,
            encrypted_text eql_v3_text_search
        )"
    );
    ddl_client.simple_query(&sql).await.unwrap();

    let client = connect_with_tls(*PROXY).await;

    let sql = format!("INSERT INTO {table} (id, encrypted_text) VALUES ($1, $2)");
    let result = client.execute(&sql, &[&id, &plaintext]).await;

    assert!(
        result.is_ok(),
        "Proxy did not reload the schema after simple-protocol DDL, \
         so the new connection cannot map {table}.encrypted_text: {:?}",
        result.err()
    );

    assert_stored_ciphertext(&table, id, &plaintext).await;

    drop_table(&ddl_client, &table).await;
}

assert_stored_ciphertext separates "the statement succeeded" from "Proxy mapped and encrypted the statement". A passthrough regression therefore cannot produce a green test.

Verification status, stated plainly: I verified the mechanism in the source, and the test compiles against d54fdfd6 (cargo check -p cipherstash-proxy-integration --tests). I could not run the test, because my local Docker has no free disk. Please run it before you act on this finding.

2. CI does not arm the memory gate

mise.toml:185, .github/workflows/test.yml:95

The task runs the soak without --max-rss-growth-mib. max_rss_growth_bytes is therefore None, and soak.rs:423-427 skips the gate. validate_report then checks only that operations > 0, that errors == 0, and that a live RSS sample exists.

The headline capability of this PR does not run in CI. The report is an artifact that somebody must read, not a gate.

The head run supplies the numbers to set a budget: rss_growth_bytes = 696320 (0.66 MiB over 30 seconds), and peak RSS 25 MiB. Please add --max-rss-growth-mib 16, or a similar value.

I raised this in review 4968785141. It is still open.

Should fix

3. A failed reload can hold a passthrough client for about 26 seconds

context/mod.rs:795-799, proxy/mod.rs:107-127, backend.rs:188

reload_schema_if_changed re-arms the flag when the reload fails. That behaviour is correct, and you tested it. The hazard is the combination of a blocking call site and a long retry ladder.

load_schema_with_retry retries 10 times, with backoff up to 2 seconds. That is about 13 seconds. proxy/mod.rs runs the schema loader and the encrypt-config loader in series, so a full failure takes about 26 seconds. The re-armed flag makes the next ReadyForQuery try again.

8661b7df puts this on the passthrough path (backend.rs:188). On main the passthrough path never reloaded. A bare-schema deployment under database connection pressure can therefore hold each ReadyForQuery for about 26 seconds, again and again, and the client sees no reason for the delay. All reloads pass through one global task, so one failing reload also delays every other connection.

Suggested fix: bound the retries at this call site, or make the passthrough reload non-blocking.

4. The report write can discard the real failure

packages/cipherstash-proxy-burn-in/src/soak.rs:119-124

write_report_atomic(...).await? runs before result?. If the write or the rename fails, the function returns the I/O error. report.terminal_error is lost with the struct, and the report file is stale or absent. CI uses if-no-files-found: warn, so the operator loses the cause on both channels.

preflight_output limits this to a failure during the run, such as a full disk. The exit code is still non-zero, so CI does not pass.

Suggested fix: log the terminal error before the write, or combine the two errors.

Nits

5. The encryption assertion does not cover the tables that the soak measures

database.rs:253-258, soak.rs:231-296

assert_seed_is_encrypted reads burnin_type_lab_samples id 1. The timed workload touches only the burnin_commerce_* tables. No at-rest ciphertext check covers a commerce column.

The encryption path is safe in practice. The eql_v3_text domain has a CHECK that rejects plaintext, and tokio_postgres rejects a String bind against the domain OID before it sends the value. A silent plaintext write cannot happen.

Two points remain:

• The assertion is close to a tautology. The domain CHECK already guarantees the keys that database.rs:264-267 tests. The assertion can find fixture DDL drift only.
• Head removed conformance from CI. conformance.rs is the only code that reads a commerce column back with typed decryption, so it is now dead in CI.

Suggested fix: assert ciphertext on one commerce row after the workload, and restore conformance to CI.

6. --help prints the fixture password

packages/cipherstash-proxy-burn-in/src/main.rs:33, :43

hide_env_values hides the environment value. It does not hide default_value. I confirmed this with --help:

--proxy-database-url <PROXY_DATABASE_URL>
    ... [env: BURN_IN_PROXY_DATABASE_URL] [default: postgresql://cipherstash:p%40ssword@localhost:6432/cipherstash]

The password is the committed local fixture credential (mise.toml:22-23), so --help discloses nothing new. But the PR body lists "redact connection credentials from help, diagnostics, and reports" as delivered. Redaction works for diagnostics and reports (DatabaseTarget, with a test at database.rs:299). It does not work for help.

Suggested fix: add hide_default_value = true, or move the defaults into a Default impl.

7. Duplicate conversion

backend.rs:187 and backend.rs:203 both compute code.into().

Withdrawn

I verified each of these against d54fdfd6 and withdraw it.

• Sticky schema_changed flag. Fixed in bc94d74e. take_schema_changed uses swap(false, AcqRel), and two tests pin the behaviour. Finding 1 above replaces it.
• migrate shares the 10-second per-operation timeout. Head uses MIGRATION_TIMEOUT. The head CI run leaves about 2.3 seconds for proxy start, migrate and teardown together, against a 10-second budget.
• timeout-minutes: 15 is too tight. The job has no proxy:up step, so it builds Proxy once. Swatinem/rust-cache is present. Measured job times are 4m23s to 5m55s, and that range includes the first run, with a cold cache.
• DROP DOMAIN ... CASCADE in tests/sql/eql-domains-uninstall.sql. This file is not in the PR. The PR changes no file under tests/sql/.
• Nested cargo build deadlock. Not a hazard. The build lock is per profile, and target/debug/.cargo-lock and target/release/.cargo-lock are separate files. cargo run also releases the lock before it runs the binary. I confirmed this: a nested release build finished in 0.24 seconds while the outer binary was running.